Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Hold a reference to the statement in a LOB stream #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NattyNarwhal
Copy link
Member

Otherwise, the statement can be freed before the LOB can be used. The strategy used is like how pdo_oci8 does it (hold ref to handle) and not like pdo_pgsql does it (hold a hashmap of handles).

Related to GH-18

Otherwise, the statement can be freed before the LOB can be used.
The strategy used is like how pdo_oci8 does it (hold ref to handle)
and not like pdo_pgsql does it (hold a hashmap of handles).

Related to GH-18
@NattyNarwhal
Copy link
Member Author

This is still a bit busted; the statement handle in SQL/CLI is still getting freed, though it no longer faults.

@NattyNarwhal
Copy link
Member Author

Looks like it gets freed before it even gets a chance to call create_lob_stream...

@NattyNarwhal
Copy link
Member Author

Sorry, I don't it gets freed; the handle number gets recycled and I think that is valid. On LUW, it returns a function sequence error. On i, that is documented, which indicates it's probably not seeking properly? Especially if create_lob_stream frobbed the handle already.

@NattyNarwhal
Copy link
Member Author

Holiday here, but writing this down from what I remember in Slack so I don't forget:

  • I don't think this code ever worked correctly (at least for LUW), even ignoring the obvious LOB resource lifetime issues. It would only work in simple cases where you get the LOB data immediately after a fetch. By the time you fetched again, the data would no longer be for that row, and fetching to the end would have nothing for you. As such, you get an obvious function sequence error (...which should be surfaced to the user, but since we're in LOB stream code, submitting PDO errors will do nothing).
  • The PASE ifdef appears to make much more sense; it uses the LOB locator. However, I haven't be able to get this to work; I don't know about the "rules" with LOB locators very well; it could be that we can't use it after fetching the next row/to the end of data. (That would seem pretty pointless, however.) It seems to complain about truncation? And why was it only marked for PASE, anyways? Why is it also allocating a new statement handle? The bookkeeping for the stream also seems funny?
  • Overall, I suspect the LOB code is really complex and hard to get right, if SQL/CLI might lack the facilities required to do this correctly (and I don't want to seek the cursor and mess with ambient state). I think the best option might be to get rid of LOB stream resources altogether and just fetch them as strings.
    • As a point of comparison, the only PDO drivers that support LOB streams are Postgres and Oracle, and they rely on APIs more intricate than SQL/CLI's for LOBs. For non-PDO, sqlite3 (why not the PDO one?) and Postgres support LOB streams. ODBC would be the closest comparison and it just returns strings, AFAIK.

@kadler
Copy link

kadler commented Feb 20, 2023

I don't know about the "rules" with LOB locators very well; it could be that we can't use it after fetching the next row/to the end of data.

Locators don't rely on the state of the cursor; they last until they are freed. Usually they are left to be implicitly freed at various execution points, eg. commit/rollback, etc. More info: https://www.ibm.com/docs/en/i/7.3?topic=objects-large-object-locators. For IBM i CLI, there's also the SQL_ATTR_FREE_LOCATORS connection attribute which can be used to close an array of locators.

FYI for IBM i Host Server jobs (ODBC, JDBC, etc which doesn't apply here AFAIK) the LOB_LOCATOR_THRESHOLD QAQQINI option determines how many locators are left open before freeing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants